Repository navigation
test(spawn): wait for background pipe closes before counting fds - #37207
Conversation
spawn-streaming-stdin.test.ts compared the highest open fd right after awaiting the last batch of subprocesses. Pipe fds that had a poll registered are closed on the work pool (`bun_io::Closer`), not on the JS thread, so on a loaded Linux machine the last batch's ~40 fds can still be open at that instant even though nothing leaked; they are gone a few milliseconds later. Poll for the count to return to the baseline (up to 1s) before asserting. No-Verification-Needed: test-only change
WalkthroughChangesSpawn file-descriptor cleanup tests
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@robobun adopt |
|
Diff is green: both spawn-streaming tests passed on every lane in builds 90615 and 90626, and all review threads are resolved. The remaining CI red is unrelated to this PR: worker-terminate-funnels.test.ts leaks on the x64-asan lane (reported separately as a main break), and the other failures recovered on retry. Ready for a maintainer. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only deflake that replaces an immediate fd-count read with a bounded 1s poll, keeping the leak assertion intact. The inline nit about spawn-streaming-stdout.test.ts is optional; that sibling hasn't been observed flaking in the cited builds.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/spawn/spawn-streaming-stdin.test.ts, changing 1 line into a 6-line bounded poll loop. The fd-leak assertion expect(newMaxFD).toBe(maxFD) is unchanged; the only difference is that newMaxFD is now re-sampled up to 100 times at 10ms intervals before asserting, to allow background bun_io::Closer work-pool tasks to finish closing the last batch's pipe fds. The PR description traces the mechanism to src/io/pipes.rs close_impl → Closer::close, reproduces the race under load (5/30 on loaded Linux, 0/30 idle, 0 on macOS — matching the CI failure distribution), and verifies 0/40 with the fix.
Security risks
None. Test-only change with no production code touched.
Level of scrutiny
Low. This is a targeted deflake of a known-flaky test with a well-understood root cause. The change follows REVIEW.md's guidance exactly: it polls a bounded window rather than sleep-then-check, the assertion still fails on a real leak (if fds never return to baseline the loop exits after 1s and the toBe fails), and the comment names the specific mechanism (background-thread pipe close) that makes an observable JS-side signal unavailable. It mirrors the expectMaxObjectTypeCount bounded-poll pattern used four lines below in the same test.
Other factors
The bug-hunting system flagged one nit: the sibling spawn-streaming-stdout.test.ts has the same getMaxFD()-after-Promise.all pattern. That's a fair observation under the "fix the whole class" rule, but it's explicitly a nit — the stdout test has a smaller race window (concurrency 7, one piped fd vs two) and hasn't appeared in the cited CI builds. It doesn't block this fix. No outstanding reviewer comments; a maintainer has already adopted the PR.
Same background close race as the stdin sibling, at both sample points: the baseline is taken right after batch 1's Promise.all, so in-flight closes can inflate it, and the final count is sampled right after the last batch. Settle-poll the baseline and poll the final count back to it before asserting.
There was a problem hiding this comment.
LGTM — test-only de-flake with a documented mechanism and reproduction; the earlier sibling-test feedback was addressed in ca399c8. The remaining inline note on the stdout baseline settle heuristic is a nit (theoretical residual, not observed, strictly narrower than pre-PR) and shouldn't block.
Extended reasoning...
Overview
Two test files touched, no production code. spawn-streaming-stdin.test.ts gains a bounded 10ms×100 poll for getMaxFD() to return to the pre-spawn baseline before asserting no fd leak — accounting for bun_io::Closer closing pipe fds on the work pool a few ms after the JS-side promises settle. spawn-streaming-stdout.test.ts gets the same final poll plus a settle loop for its post-batch-1 baseline capture, applied in ca399c8 after the previous review round.
Security risks
None. Test-only; no user-facing surface, no untrusted input handling.
Level of scrutiny
Low-to-medium. This is a de-flake of an fd-leak assertion in test code. The key REVIEW.md concern for de-flakes — "keep asserting the property the original assertion protected" — is satisfied: the assertion is still exact .toBe(maxFD), only preceded by a bounded poll, mirroring how expectMaxObjectTypeCount immediately below already works. The stdin fix is clean because its baseline is captured before any spawn (a true target). The stdout settle heuristic has a theoretical false-positive window flagged in the inline nit, but it is strictly better than the previous immediate sample, wasn't flaking in the cited builds, and passed 8/8 under the 2×nproc load reproduction.
Other factors
The PR description traces the mechanism to source (src/io/pipes.rs close_impl → Closer::close), reproduced under load (5/30 → 0/40), and verified under debug+ASAN. The earlier review comment (cover the sibling stdout test) was addressed and the thread resolved. The remaining nit is optional polish; approving with it attached.
The baseline is a settle heuristic with no pre-spawn ground truth (#6724 moved it after the first batch on purpose), and getMaxFD is a max, so unchanged-for-10ms can still lock in a value above the true floor. Poll the final count down only while it is above the baseline and assert <=; a count below the baseline can never be a leak.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/spawn/spawn-streaming-stdout.test.ts`:
- Around line 43-52: Replace getMaxFD-based stabilization in
test/js/bun/spawn/spawn-streaming-stdout.test.ts lines 43-52 with a
pipe-specific completion check using an open-descriptor count/set or equivalent
observation that detects all pipe closures; retain the relaxed assertion at
lines 55-64 only when the measurement cannot conceal lower-numbered leaks. Apply
the same complete cleanup observation to
test/js/bun/spawn/spawn-streaming-stdin.test.ts lines 66-72, keeping the
existing test behavior otherwise unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2efabc24-2572-4bcd-822d-584abfdfa8d4
📒 Files selected for processing (2)
test/js/bun/spawn/spawn-streaming-stdin.test.tstest/js/bun/spawn/spawn-streaming-stdout.test.ts
There was a problem hiding this comment.
LGTM — both earlier review points (stdout sibling, high-settled baseline) are addressed in bf2c2d3.
What was reviewed:
- stdin: bounded poll toward the pre-spawn baseline, exact
toBeassertion unchanged — still catches a real leak after the 1 s window. - stdout: final poll now conditions on
> maxFDand asserts<=, so a settle-high baseline can't false-fail; a real per-batch leak still exceeds any post-batch-1 baseline and fails. - Checked that
<=doesn't weaken beyond what the #6724 post-batch-1 baseline already allowed, and that the added polls are ~10 ms in the common case.
Extended reasoning...
Overview
Test-only deflake touching two spawn streaming tests. The stdin test adds a bounded poll (100 × 10 ms) before comparing getMaxFD() to a pre-spawn baseline, because pipe fds are closed asynchronously via bun_io::Closer on the work pool and can lag the JS thread by a few ms under load. The stdout sibling gets the same treatment plus a settle poll for its post-batch-1 baseline, with the final assertion relaxed to toBeLessThanOrEqual since that baseline is a heuristic with no pre-spawn ground truth (deliberately moved after batch 1 in #6724).
Security risks
None. Test-only; no production code, no new inputs, no external network.
Level of scrutiny
Low. This is a CI flake fix with a well-identified mechanism (src/io/pipes.rs close_impl → Closer::close), reproduced under load (5/30 flake → 0/40 with the poll), and verified 8/8 rounds under 2×nproc load plus debug ASAN. The poll pattern mirrors expectMaxObjectTypeCount already used a few lines below.
Other factors
Two prior review rounds from this bot were both addressed and resolved: (1) the stdout sibling was covered in ca399c8, (2) the settle-high baseline / equality-target concern was fixed in bf2c2d3 by switching the final poll to > maxFD and the assertion to <=. I re-checked that <= still protects the original invariant: a real leak accumulates across ~15 batches and will exceed any plausibly-inflated post-batch-1 baseline, and the expectMaxObjectTypeCount checks below still guard object leaks. The stdin test keeps its exact toBe since its baseline is captured before any spawn. The added polls cost ~10 ms in the common (idle) case and are bounded at 1 s. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — both prior review rounds are addressed and the current diff is a clean test-only deflake.
What was reviewed:
- stdin test: bounded 1 s poll before the exact
toBe(maxFD)against a true pre-spawn baseline — assertion strength preserved. - stdout test: settle-poll for the post-batch-1 baseline, final poll while
> maxFD, assert<=; the relaxation is justified in-comment (#6724 moved the baseline on purpose, no pre-spawn ground truth) and a real leak compounds across 15 batches so still fails. - Poll shape mirrors
expectMaxObjectTypeCountdirectly below; comments name the mechanism (bun_io::Closerbackground close), satisfying the REVIEW.md sleep-in-poll rule.
Extended reasoning...
Overview
Test-only deflake of two spawn fd-leak assertions. spawn-streaming-stdin.test.ts adds a bounded poll (10 ms × 100) before asserting newMaxFD === maxFD against a pre-spawn baseline. spawn-streaming-stdout.test.ts adds a settle-poll for its post-batch-1 baseline plus a final poll that runs only while the count is above baseline, and relaxes the assert to toBeLessThanOrEqual. No production code touched.
Security risks
None. Test-only; no auth, crypto, network, or user-input handling changed.
Level of scrutiny
Low-to-medium. The mechanism (pipe fds closed by bun_io::Closer on the work pool after streams finish from JS's view) is traced to source in the PR description, reproduced under 2×nproc load (5/30 → 0/40), and verified under debug ASAN + 8/8 loaded release rounds. The main REVIEW.md concern for deflakes — "keep asserting the property the original assertion protected" — is met: stdin keeps exact toBe against a true pre-spawn baseline; stdout's <= relaxation is justified because its baseline was deliberately moved after batch 1 in #6724 (no pre-spawn ground truth), and a real per-subprocess leak compounds across ~15 batches × 7 procs so would land far above any settled baseline. Exact-level detection remains in the stdin half of the pair.
Other factors
Two prior review rounds from me were both applied (ca399c8 covered the stdout sibling; bf2c2d3 changed the final poll to > + <= so a high-settled baseline can't fail it). CodeRabbit's suggestion to redesign around a pipe-specific fd count was declined with sound reasoning (no JS-visible completion signal, cross-platform cost, leak still compounds above baseline) and withdrawn. All inline threads are resolved. The only commit since bf2c2d3 is a CI retrigger. The poll pattern matches expectMaxObjectTypeCount used immediately below in both files, and the in-code comments name the background-close mechanism per the REVIEW.md rule on sleeps inside bounded polls.
There was a problem hiding this comment.
LGTM — bf2c2d3 closes the residual inflated-baseline mode from my last comment; all threads resolved.
What was reviewed:
- stdin test: poll is bounded (100×10 ms) toward a pre-spawn ground-truth target; the exact
toBeassertion is unchanged, so leak detection strength is preserved. - stdout test:
<=weakening is justified — baseline is a post-batch-1 heuristic (#6724) with no ground truth, and a real leak compounds across ~15 batches so lands well above it; the stdin sibling keeps exact detection. - Poll shape matches
expectMaxObjectTypeCountalready used in both files; comments cite the mechanism (bun_io::Closer) and #6724.
Extended reasoning...
Overview
Test-only deflake touching two spawn streaming tests. Both previously read getMaxFD() immediately after await Promise.all(...) on the last subprocess batch and asserted equality to a baseline. Pipe fds with a registered poll are closed by bun_io::Closer on the work pool, not the JS thread, so under load the last batch's fds can still be open at that instant. The fix adds a bounded poll (10 ms steps, ≤1 s) before the assertion — the same pattern expectMaxObjectTypeCount already uses a few lines below in both files.
The stdin test's assertion is unchanged (toBe against a true pre-spawn baseline). The stdout test's baseline is captured after batch 1 (deliberately, per #6724), so it now settle-polls that baseline and asserts toBeLessThanOrEqual at the end, since a max-based settle can rarely stop slightly high and a count below a heuristic baseline cannot indicate a leak.
Security risks
None. Test-only; no runtime code, no external inputs.
Level of scrutiny
Low-medium. This is a CI flake fix that only relaxes when the fd count is sampled (bounded poll), not what is asserted, for the stdin test. For the stdout test the assertion is loosened from toBe to toBeLessThanOrEqual, which triggers REVIEW.md's "never silently weaken an existing test" scrutiny — but the weakening is explicit, commented with the reason and the PR that made the baseline a heuristic (#6724), and the property under test (no fd leak) is still protected because a real leak compounds across ~105 subprocesses and would land far above any settled baseline. The stdin test retains exact toBe against a pre-spawn ground truth, so exact-level detection stays in the pair.
Other factors
This PR has already been through two rounds of my own review (sibling coverage → ca399c8; residual inflated-baseline mode → bf2c2d3), both applied and verified under load + debug ASAN. CodeRabbit's suggestion to redesign the measurement was declined with a sound rationale (no JS-visible completion signal; getMaxFD is the established cross-platform harness helper) and withdrawn. robobun reports both tests green on all lanes in builds 90615/90626 with remaining CI red unrelated. dylan-conway adopted the PR. Nothing outstanding.
…n-sh#37207) ### What does this PR do? Fixes the `test/js/bun/spawn/spawn-streaming-stdin.test.ts` flake (debian x64/aarch64, builds 89694, 89561, 88187): ``` 68 | // assert we didn't leak any file descriptors 69 | expect(newMaxFD).toBe(maxFD); Expected: 21 Received: 56 ``` Nothing is leaked; the test samples too early. After `await Promise.all(proms)` for the last batch of 16 subprocesses, every stream has finished from JS's point of view (stdout EOF, `stdin.end()` resolved, `exited` resolved), but a pipe fd that had a poll registered is closed by `bun_io::Closer` on the work pool rather than on the JS thread (`src/io/pipes.rs` `close_impl` → `Closer::close`, by design so a blocking `close()` never stalls the loop). On a loaded machine those pool tasks can lag the JS thread by a few ms, so the last batch's stdin/stdout/pidfd descriptors (~44) are still open at the instant `getMaxFD()` runs. Reproduced on Linux x64 with 2×nproc busy loops running: 5 of 30 rounds of the same spawn/await/count loop saw `maxFD 7 -> 51` immediately after the await and `7` again 10 ms later; 0/30 idle, and macOS didn't reproduce at all (which matches the CI distribution). The assertion is unchanged; it now polls for the count to return to the baseline (10 ms steps, up to 1 s) before asserting, the same way `expectMaxObjectTypeCount` right below it already waits for object counts. With that loop the loaded Linux reproduction is 0/40. Review pointed out that `spawn-streaming-stdout.test.ts` has the same shape, at both sample points: the final count is read right after the last batch's `Promise.all`, and the baseline is read right after batch 1's, so in-flight closes can inflate the baseline as well (a failure the final poll alone can't recover from). That file now settle-polls the baseline, then polls the final count down only while it is above the baseline and asserts `<=` instead of exact equality: the baseline there is a heuristic with no pre-spawn ground truth (46a337c in oven-sh#6724 moved it after the first batch on purpose), so a count below it is not a leak, and a max-based settle check can rarely stop a little high. The stdin test keeps its exact `toBe` since its baseline is taken before any spawn. ### How did you verify your code works? Reproduction/verification above (release binary on Linux); `bun bd test test/js/bun/spawn/spawn-streaming-stdin.test.ts` passes locally. Both test files also pass under a debug ASAN build and 8/8 rounds each under 2×nproc CPU load on the release binary. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 3 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/bun/spawn/spawn-streaming-stdin.test.ts' 'test/js/bun/spawn/spawn-streaming-stdout.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/spawn/spawn-streaming-stdin.test.ts test/js/bun/spawn/spawn-streaming-stdout.test.ts bun test v1.4.0 (10c944d) test/js/bun/spawn/spawn-streaming-stdout.test.ts: { objects: { "Array Iterator": 1, Array: 85, ArrayBuffer: 2, AsyncDisposableStack: 1, AsyncFromSyncIterator: 1, AsyncFunction: 111, AsyncFunctionGenerator: 14, AsyncGenerator: 1, AsyncGeneratorFunction: 2, AsyncIterator: 1, BigInt: 3, BigInt64ArrayPrototype: 1, BigUint64ArrayPrototype: 1, Blob: 1, Boolean: 1, Buffer: 1, Bun: 1, Callee: 3, "Cell Butterfly": 32, CustomEvent: 2, CustomGetterSetter: 83, DOMAttributeGetterSetter: 89, Dirent: 2, DisposableStack: 1, Error: 6, ErrorCodeCache: 1, Event: 2, EventEmitter: 1, EventTarget: 2, Expect: 1, ExpectStatic: 1, ExpectTypeOf: 1, FileInternalReadableStreamSource: 14, FileSink: 3, FinalizationRegistry: 1, Float16ArrayPrototype: 1, Float32ArrayPrototype: 1, Float64ArrayPrototype: 1, FrameworkFileSystemRouter: 1, FullPromiseReaction: 15, Function: 2668, FunctionCodeBlock: 135, FunctionExecutable: 1441, FunctionRareData: 264, Generator: 1, GeneratorFunction: 2, GetterSetter: 191, GlobalObject: 1, ImportMeta: 3, Int16ArrayPrototype: 1, Int32ArrayPrototype: 1, Int8Array: 1, Int8ArrayPrototype: 1, InternalFieldTuple: 24, InternalModuleRegistry: 1, Intl: 1, "Iterator Helper": 1, Iterator: 2, JSAsyncGeneratorFunction: 8, JSGlobalLexicalEnvironment: 1, JSGlobalProxy: 1, JSLexicalEnvironment: 241, JSModuleEnvironment: 11, JSON: 1, JSPropertyNameEnumerator: 5, JSSourceCode: 11, "Map Iterator": 1, Map: 6, Math: 1, Module: 3, ModuleLoader: 1, ModuleNamespaceObject: 4, ModuleProgramCode ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/bun/spawn/spawn-streaming-stdin.test.ts | 8 +++++++- test/js/bun/spawn/spawn-streaming-stdout.test.ts | 21 +++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) ``` </details> **gate history** · 4 passed · 0 rejected · iteration 3 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/bun/spawn/spawn-streaming-stdin.test.ts 1 0 0 test/js/bun/spawn/spawn-streaming-stdout.test.ts 1 2 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
What does this PR do?
Fixes the
test/js/bun/spawn/spawn-streaming-stdin.test.tsflake (debian x64/aarch64, builds 89694, 89561, 88187):Nothing is leaked; the test samples too early. After
await Promise.all(proms)for the last batch of 16 subprocesses, every stream has finished from JS's point of view (stdout EOF,stdin.end()resolved,exitedresolved), but a pipe fd that had a poll registered is closed bybun_io::Closeron the work pool rather than on the JS thread (src/io/pipes.rsclose_impl→Closer::close, by design so a blockingclose()never stalls the loop). On a loaded machine those pool tasks can lag the JS thread by a few ms, so the last batch's stdin/stdout/pidfd descriptors (~44) are still open at the instantgetMaxFD()runs. Reproduced on Linux x64 with 2×nproc busy loops running: 5 of 30 rounds of the same spawn/await/count loop sawmaxFD 7 -> 51immediately after the await and7again 10 ms later; 0/30 idle, and macOS didn't reproduce at all (which matches the CI distribution).The assertion is unchanged; it now polls for the count to return to the baseline (10 ms steps, up to 1 s) before asserting, the same way
expectMaxObjectTypeCountright below it already waits for object counts. With that loop the loaded Linux reproduction is 0/40.Review pointed out that
spawn-streaming-stdout.test.tshas the same shape, at both sample points: the final count is read right after the last batch'sPromise.all, and the baseline is read right after batch 1's, so in-flight closes can inflate the baseline as well (a failure the final poll alone can't recover from). That file now settle-polls the baseline, then polls the final count down only while it is above the baseline and asserts<=instead of exact equality: the baseline there is a heuristic with no pre-spawn ground truth (46a337c in #6724 moved it after the first batch on purpose), so a count below it is not a leak, and a max-based settle check can rarely stop a little high. The stdin test keeps its exacttoBesince its baseline is taken before any spawn.How did you verify your code works?
Reproduction/verification above (release binary on Linux);
bun bd test test/js/bun/spawn/spawn-streaming-stdin.test.tspasses locally. Both test files also pass under a debug ASAN build and 8/8 rounds each under 2×nproc CPU load on the release binary.[stamp-90s] gate passed · iteration 3 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 3
evidence per changed file